Skip to content

gh-128627: Use __builtin_wasm_test_function_pointer_signature for Emscripten trampoline #137470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Aug 6, 2025

Since llvm/llvm-project#150201 was merged, there is now a better way to do this. Requires Emscripten 4.0.12. This lets us get rid of all the hand encoded webassembly.

@freakboy3742 this is blocked on updating the buildbot to use Emscripten 4.0.12, not sure what that entails.

…or Emscripten trampolines

Since llvm/llvm-project#150201 was merged,
there is now a better way to do this. Requires Emscripten 4.0.12.
@hoodmane hoodmane force-pushed the __builtin_wasm_test_function_pointer_signature branch from 6bbee1e to 0ffb76b Compare August 6, 2025 15:41
@hoodmane hoodmane requested a review from freakboy3742 August 6, 2025 15:42
@hoodmane
Copy link
Contributor Author

hoodmane commented Aug 6, 2025

!buildbot emscripten

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @hoodmane for commit 0ffb76b 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137470%2Fmerge

The command will test the builders whose names match following regular expression: emscripten

The builders matched are:

  • WASM Emscripten PR

@hoodmane
Copy link
Contributor Author

hoodmane commented Aug 6, 2025

!buildbot emscripten

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @hoodmane for commit 04e0541 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137470%2Fmerge

The command will test the builders whose names match following regular expression: emscripten

The builders matched are:

  • WASM Emscripten PR

@@ -3106,6 +3106,11 @@ config.status: $(srcdir)/configure
Python/asm_trampoline.o: $(srcdir)/Python/asm_trampoline.S
$(CC) -c $(PY_CORE_CFLAGS) -o $@ $<

Python/emscripten_trampoline_inner.wasm: $(srcdir)/Python/emscripten_trampoline_inner.c
$$(dirname $$(dirname $(CC)))/bin/clang -o $@ $< -mgc -O2 -Wl,--no-entry -Wl,--import-table -Wl,--import-memory -target wasm32-unknown-unknown -nostdlib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow what the "dirname of dirname" is doing here - doesn't this assume that emcc (which should be the value of CC) is installed in /usr/bin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emcc has a path like emsdk/upstream/emscripten/emcc and we're looking for emsdk/upstream/bin/clang.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment explaining.

@freakboy3742
Copy link
Contributor

There's three blockers to this at present:

  1. The check-c-globals analyser check that is failing CI on this PR
  2. The test suite CI failure that is also visible on main, because of the change from gh-131876: extract _hashlib helpers into a separate directory #136995. gh-131876: extract _hashlib helpers into a separate directory #137319 looks like a possible fix; however, it looks like it's on hold pending some other changes.
  3. The refactoring of the build script that we discussed at EuroPython (and on the buildmaster-config PR) to encode the Emscripten version as part of the build script, and use an argument to point at a local cache directory.

The alternative to (3) would be to violate the "RC1 is where we lock in the Emscripten version" rule, and bump to 4.0.12. That isn't an approach we could take in the general case, but given this is the first "back to Tier 3" release, and there's no impact on downstream packagers (since nobody can publish to PyPI, so nobody will be relying on the 4.0.11 ABI guarantee), I could probably be convinced to do this - but I'd like @hugovk to sign off on the plan as release manager.

Of course (3) is going to be needed in the fullness of time - it's just a question of whether we need to do it right now. And (1) and (2) will need a fix regardless.

Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as "needs changes" - see previous comment.

@bedevere-app
Copy link

bedevere-app bot commented Aug 6, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hoodmane
Copy link
Contributor Author

hoodmane commented Aug 7, 2025

This PR isn't urgent so I think it would be fine to block it on making an Emscripten version manager. But it would also be fine to bump the Emscripten version for 3.14 imo.

@hoodmane hoodmane requested a review from AA-Turner as a code owner August 7, 2025 08:41
@picnixz
Copy link
Member

picnixz commented Aug 7, 2025

I reverted the hashlib refactorization by the way, so failures on main shouldn't be because of this.

@hoodmane
Copy link
Contributor Author

hoodmane commented Aug 7, 2025

Thanks @picnixz. Failure on main is a runtime failure in a test about calendars and locales, not the linker error.

@hoodmane
Copy link
Contributor Author

hoodmane commented Aug 7, 2025

Hmm the C parser doesn't work with out of tree builds because it passes a bunch of invalid paths like -I/home/rchatham/cpython/*./Include cc @ericsnowcurrently
https://github.com/python/cpython/blob/main/Tools/c-analyzer/cpython/_parser.py?plain=1#L120

@hoodmane
Copy link
Contributor Author

hoodmane commented Aug 7, 2025

I can't reproduce the NotImplementedError locally:

NotImplementedError: (Struct(file=FileInfo(filename='/home/runner/work/cpython/cpython/Include/pystate.h', lno=62), name='_stack_chunk', data=[Member(name='previous', vartype=VarType(typequal=None, typespec='struct _stack_chunk', abstract='*'), size=None), Member(name='size', vartype=VarType(typequal=None, typespec='size_t', abstract=''), size=None), Member(name='top', vartype=VarType(typequal=None, typespec='size_t', abstract=''), size=None), Member(name='data', vartype=VarType(typequal=None, typespec='PyObject', abstract='* [1]'), size=None)], parent=None), [TypeDef(file=FileInfo(filename='/home/runner/work/cpython/cpython/Include/pytypedefs.h', lno=18), name='PyObject', data=VarType(typequal=None, typespec='struct _object', abstract=''), parent=None), TypeDef(file=FileInfo(filename='/home/runner/work/cpython/cpython/Python/emscripten_trampoline_inner.c', lno=1), name='PyObject', data=VarType(typequal=None, typespec='void', abstract=''), parent=None)])

@freakboy3742
Copy link
Contributor

I reverted the hashlib refactorization by the way, so failures on main shouldn't be because of this.

My apologies - I missed that the cause of the CI failures on main had changed. Thanks for the revert - now to work out what's going on with Calendars... :-)

@hugovk
Copy link
Member

hugovk commented Aug 13, 2025

The alternative to (3) would be to violate the "RC1 is where we lock in the Emscripten version" rule, and bump to 4.0.12. That isn't an approach we could take in the general case, but given this is the first "back to Tier 3" release, and there's no impact on downstream packagers (since nobody can publish to PyPI, so nobody will be relying on the 4.0.11 ABI guarantee), I could probably be convinced to do this - but I'd like @hugovk to sign off on the plan as release manager.

@freakboy3742 Yes, I think it's okay to bump the Emscripten version given what you say.

@freakboy3742
Copy link
Contributor

@freakboy3742 Yes, I think it's okay to bump the Emscripten version given what you say.

Thanks - I've just updated the buildbot to use 4.0.12.

@freakboy3742
Copy link
Contributor

!buildbot emscripten

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 6fd7bfe 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137470%2Fmerge

The command will test the builders whose names match following regular expression: emscripten

The builders matched are:

  • WASM Emscripten PR

@freakboy3742
Copy link
Contributor

@hoodmane The buildbot is now running 4.0.12, but it's definitely not happy. The problem seems to be back in HACL - but I've merged with main, so it's not the same problem as before (and the problem doesn't exist on main right now).

@hoodmane
Copy link
Contributor Author

hoodmane commented Aug 15, 2025

It looks to me like the build failure is due to buggy code in this PR. Which is good news =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants